Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

3rd party modules updater for updatenotification #3150

Merged
merged 31 commits into from
Nov 10, 2023
Merged

3rd party modules updater for updatenotification #3150

merged 31 commits into from
Nov 10, 2023

Conversation

bugsounet
Copy link
Contributor

@bugsounet bugsounet commented Jul 14, 2023 •
edited
Loading

#3142

Added my (modified) updater main core into updatenotification default module

Missing: callback display in MM² (i will code it after)

new part of configuration added:

		updates: [
			// array of module update commands
			{
				// with embed npm script
				"MMM-Test": "npm run update"
			},
			{
				// with "complex" process
				"MMM-OtherSample": "rm -rf package-lock.json && git reset --hard && git pull && npm install"
			},
			{
				// with git pull && npm install
				"MMM-OtherSample2": "git pull && npm install"
			},
			{
				// with a simple git pull
				"MMM-OtherSample3": "git pull"
			}
		],
		updateTimeout: 2 * 60 * 1000, // max update duration
		updateAutorestart: false // autoRestart MM when update done ?

@khassel: i need your help
I don't use docker, maybe you can help me for this:
How can i check if MM² is running inside a docker ? (from MM² main core)
Actually, I check if we use pm2 or not.
I have to check if docker is used or not too
last time you tell me: "you can't use updater with docker", so I want to check and deny any update if docker used

@bugsounet
Copy link
Contributor Author

humm... I don't want to push new package-lock now but it's needed

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023 •
edited
Loading

Codecov Report

Merging #3150 (b7dffce) into develop (66b29ec) will increase coverage by 0.48%.
Report is 1 commits behind head on develop.
The diff coverage is 39.71%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop    #3150      +/-   ##
===========================================
+ Coverage    25.19%   25.68%   +0.48%     
===========================================
  Files           54       55       +1     
  Lines        11933    12202     +269     
===========================================
+ Hits          3007     3134     +127     
- Misses        8926     9068     +142
Files Coverage Δ
modules/default/calendar/calendar.js 0.00% <ø> (ø)
modules/default/updatenotification/node_helper.js 78.84% <50.00%> (+5.51%) ⬆️
...s/default/updatenotification/updatenotification.js 0.00% <0.00%> (ø)
...odules/default/updatenotification/update_helper.js 45.08% <45.08%> (ø)

... and 1 file with indirect coverage changes

@bugsounet
Copy link
Contributor Author

I think that pm2 will correct Vulnerabilities soon

@khassel
Copy link
Collaborator

khassel commented Jul 14, 2023

How can i check if MM² is running inside a docker ? (from MM² main core)
last time you tell me: "you can't use updater with docker", so I want to check and deny any update if docker used

are you talking of restarting? Because updater in general has no problem with docker ...

Simple method is to check if the file /.dockerenv exists.

@bugsounet
Copy link
Contributor Author

maybe it's better to use 'pm2 prettylist' with child_process exec function instead of the pm2 library.
It will correct Vulnerabilities, no ?
because I think they are in no hurry to fix this

@bugsounet
Copy link
Contributor Author

ah... how solve this translations_spec.js error ?

I have added some new translations part in en and fr json file
I'm not able to translate all ;) ;)

what's missing ?

@bugsounet
Copy link
Contributor Author

bugsounet commented Aug 2, 2023 •
edited
Loading

Ok i found: de json file is considered as default (I thought it was 'en')
I will see with my de translator :)

@khassel
Copy link
Collaborator

khassel commented Aug 2, 2023

Ok i found: de json file is considered as default (I thought it was 'en')

yes, there was a reason to switch but I forgot it ...

I will see with my de translator :)

can review this before merge ...

@bugsounet
Copy link
Contributor Author

yes @khassel : maybe this

@bugsounet
Copy link
Contributor Author

It's not totaly Done:

  • I have to Clean some not needed parts
  • make Class Docs

@bugsounet bugsounet requested a review from rejas September 1, 2023 11:35
@bugsounet
Copy link
Contributor Author

@rejas: It's ok for me for review

Can you restart again test suite because ... as usual "ReferenceError: moment is not defined" error is again there ...
thx ;)

@bugsounet
Copy link
Contributor Author

Ah the misery, ... I hate resolving a conflict in the package-lock.json

I'll leave it like this for now, we'll fix it when we push it

You have to test anyway

@khassel khassel mentioned this pull request Sep 9, 2023
Closed
@bugsounet
Copy link
Contributor Author

@rejas:

will we put it in place for v2.26.x?
I don't think we will have time to do the tests/review and write the doc

@rejas
Copy link
Collaborator

rejas commented Sep 11, 2023 •
edited
Loading

Next version is 2.25, so lets move it to v2.26, ok?

@rejas rejas added this to the 2.26 milestone Sep 11, 2023
@bugsounet
Copy link
Contributor Author

@rejas

Hi, this seems ready for review

@khassel
Copy link
Collaborator

khassel commented Oct 7, 2023

concerning running as docker container: Restarting should work with this code (if container is set to restart automatically), will test this after this is merged on develop.

@bugsounet
Copy link
Contributor Author

This test suite is very random :/
it works half the time locally...

Test Suites: 40 passed, 40 total
Tests:       574 passed, 574 total
Snapshots:   9 passed, 9 total

ping: @rejas

what do we do with this PR?
Merge it or review again ;)

@bugsounet
Copy link
Contributor Author 

ping @rejas
ping: connect: Network is unreachable

rejas
rejas approved these changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets see how it behaves in develop :-)

@rejas
Copy link
Collaborator

rejas commented Nov 9, 2023 •
edited
Loading

could you bring it up to the latest develop so we see the tests become green? thx for your patience!

@rejas rejas merged commit 203e864 into MagicMirrorOrg:develop Nov 10, 2023
5 checks passed
@khassel
Copy link
Collaborator

khassel commented Nov 20, 2023

@bugsounet I was looking for the new restart config option for updatenotification but couldn't find this in the documentation repo (maybe I'm blind).

So can you please provide a Documentation-PR for the new features you implemented here? Thanks!

@bugsounet
Copy link
Contributor Author

It's planned ;)
I will do it before release, don't worry ;)

@bugsounet bugsounet deleted the updates-notification branch December 1, 2023 12:32
@bugsounet
Copy link
Contributor Author

Finally... I'm not sure that I will have time to write it before release :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khassel khassel khassel approved these changes

@rejas rejas rejas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.26
Development

Successfully merging this pull request may close these issues.
4 participants
@bugsounet @codecov-commenter @khassel @rejas